-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Refactor to more idiomatic Jaspr code #12726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @schultek, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on refactoring several UI elements to align with more idiomatic Jaspr component patterns. The primary goal is to reduce manual DOM manipulation in client-side scripts by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is an excellent refactoring that significantly improves the codebase. Moving logic from global_scripts.dart into self-contained Jaspr components like Tooltip and PageNav is a huge win for maintainability and follows best practices for component-based architecture. The new ComponentRef system is a clever solution to work around the current limitations of Jaspr for passing server-rendered components to client components. The updated Dropdown API is also much cleaner and more intuitive. Overall, these changes make the code more idiomatic, robust, and easier to work with. I have one suggestion to improve the Dropdown component's behavior.
7d21efe to
85d64c4
Compare
|
Visit the preview URL for this PR (updated for commit 8369e3e): https://flutter-docs-prod--pr12726-feat-cleanup-jaspr-code-zmihxxot.web.app |
parlough
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making that fix! Looks great to me :D
This refactors some parts of the code to have more idiomatic Jaspr components and reduce logic in
global_scripts.dart.ComponentRefsystem that in some capacity works similar to server components (more limited and less elegant, but functionally working). This is to unblock some improvements (some of which are included in this PR) to the interactive components of the website, as we can already start architecting towards server components and write less manual dom logic.Dropdowncomponent to accept directtoggleandcontentcomponents, which simplifies it's implementation. Migrate existing uses ofDropdown.PageNav) to use theDropdowncomponent, as well as the newComponentRefs. Split into separate "site-subheader" and "pagenav" elements. Preparations for Add optional page navigation to top toc dropdown (for FWE) #12715Tooltipcomponent and migrate the glossary tooltips to it using the newComponentRefs. Can be reused for feat: add a tooltip for each api docs link #12600